Recently I had the opportunity to speak with Ehsan and Smaug about how PrivateBrowsing is implemented and how we can unify it with OriginAttributes.userContextId. The basic idea is to add a inMemory attribute to OriginAttributes, so that QuotaManager (and some other components) can provide an in-memory support that doesn't require any I/O and that is not persistent. Once we have this, PrivateBrowsing can be implemented in the Firefox frontend as a custom userContextId + inMemory flag. Similar implementation can be used for the Presentation API. The idea is simple, but it requires many changes to many components of gecko. First of all QuotaManager: instead working with files and directories, it must provide mozStorage connections and a custom nsIFile implementation, so that the rest of gecko doesn't need to know the real 'backend' of such files and dbs.

It's not clear to me why this needs to be a separate OriginAttribute. Can't we just have a predicate |bool UserContextMayPersist(userContextId)| or something?

Conceptually, each OA is an orthogonal dimension in the origin space. So if it doesn't make sense for there to be both { userContextId: X, inMemory: true } and { userContextId: X, inMemory: false } for the same value of X, then inMemory is probably not the correct thing to use.

(In reply to Bobby Holley (busy) from comment #2) > Conceptually, each OA is an orthogonal dimension in the origin space. So if PrivateBrowsing at the moment runs with userContextId 0 but it uses a similar concept of "in-memory attribute" so that it doesn't touch the persistent data. The same concept should work for any kind of userContextIds. I could want to use containers in private browsing as instance (read - multiple private browsing). In theory we can say: even userContextIds are persistent, and odds are in-memory. That would work as well. But if we follow this approach, in order to create a private browsing session, we need to use a nsIUserContextService for having the 'next' available userContextId. Otherwise, if the in-memory attribute is in OriginAttributes, we can set that value to true, and we are in a non-persistent state of the same userContextId.

(In reply to Andrea Marchesini (:baku) from comment #3) > (In reply to Bobby Holley (busy) from comment #2) > > Conceptually, each OA is an orthogonal dimension in the origin space. So if > > PrivateBrowsing at the moment runs with userContextId 0 but it uses a > similar concept of "in-memory attribute" so that it doesn't touch the > persistent data. The same concept should work for any kind of > userContextIds. I could want to use containers in private browsing as > instance (read - multiple private browsing). My suggestion here is to have some external function we can call to ask the persistence policy on a given userContextId. This would allow multiple private browsing. > > In theory we can say: even userContextIds are persistent, and odds are > in-memory. That would work as well. I don't parse this. > But if we follow this approach, in order to create a private browsing > session, we need to use a nsIUserContextService for having the 'next' > available userContextId. Otherwise, if the in-memory attribute is in > OriginAttributes, we can set that value to true, and we are in a > non-persistent state of the same userContextId. Yes, we would need an ambiently-available service to map userContextIds to persistence policies. IIUC the initialization of the different contexts would still happen in the frontend, and could use whatever data structures are being used already.

I agree that ideally the attributes in OriginAttributes should be orthogonal. But are for example userContextId and addonId orthogonal? I.e. do we ever create principals with both set? I think it might make sense to add a |unsigned long privateBrowsingId| to OriginAttributes. I think this would be ok to do even if in practice we only ever set one of "userContextId", "addonId" and "privateBrowsingId" at once. Eventually I think we could look at combining unsigned long userContextId; DOMString addonId; unsigned long privateBrowsing; ... more stuff ... into something like DOMString extraType; DOMString extraValue; where 'extraType' is set to "userContext", "addon" or "privateBrowsing". That would make the attributes truly orthogonal.

That said, I don't think we should use privateBrowsingId as additional information. We should use it instead of the way we currently track private browsing information. Which is no small project.

(In reply to Jonas Sicking (:sicking) from comment #5) > I agree that ideally the attributes in OriginAttributes should be > orthogonal. But are for example userContextId and addonId orthogonal? I.e. > do we ever create principals with both set? Probably not, but they still represent two relatively-different concepts. And in general, I don't think the existence of one questionable usage is justification for additional ones. > I think it might make sense to add a |unsigned long privateBrowsingId| to > OriginAttributes. I think this would be ok to do even if in practice we only > ever set one of "userContextId", "addonId" and "privateBrowsingId" at once. Can you explain why this would be preferable to just using userContext for PB and adding a predicate to answer the question of whether it's persistent? They seem to be basically identical concepts modulo that, and it seems to me that we would benefit from handling them with the same code.

Because it seems like it'd create hacky solutions throughout the rest of the codebase. Any time we wanted to know if a page is in private-browsing mode, we'd have to look up in a separate hashtable tracking which userContextIds are private-browsing ones, and which ones aren't. This is needed when we turn off link coloring, or when we decide to store cookies in-memory-only. This hashtable will need to be synced across all relevant processes so that we can answer the question synchronously without performance impact. It also means that in the databases where we track "real" user contexts, we have to add exceptions to handle any time someone is querying information about a userContextId which is a private-browsing one. Another effect here might be that it now becomes possible to see how many different private-browsing sessions have existed by looking at gaps in saved userContextIds. What is the advantage of reusing userContextId here?

(In reply to Jonas Sicking (:sicking) from comment #8) > Because it seems like it'd create hacky solutions throughout the rest of the > codebase. To me it seems quite the opposite. > Any time we wanted to know if a page is in private-browsing mode, we'd have > to look up in a separate hashtable tracking which userContextIds are > private-browsing ones, and which ones aren't. You're making this sound onerous, but it's not. We have a function that abstracts this detail away for us. And it doesn't need to be a hashtable - at the bare minimum, it's just a list of userContextIds that are non-persistent, which will usually have zero entries, occasionally one entry, and seldom more. > This is needed when we turn off link coloring Don't we want link color to also be user-context-aware? This seems like a case that would benefit from more-unified handling, though I understand that we also have an additional more-stringent policy for PB that goes above and beyond simply segmenting the histories. > This hashtable will need to be synced across all relevant processes so that > we can answer the question synchronously without performance impact. This isn't hard either. We have lots of similar stuff. > It also means that in the databases where we track "real" user contexts, we > have to add exceptions to handle any time someone is querying information > about a userContextId which is a private-browsing one. So this might be the heart of the misunderstanding. I'm suggesting that, from the perspective of the platform, they are basically the same thing. PB isn't a "fake" user context - it's a regular user-context with a few additional directives like "don't persist" and "disable visited highlighting". > Another effect here might be that it now becomes possible to see how many > different private-browsing sessions have existed by looking at gaps in saved > userContextIds. This depends entirely on how the frontend decides to allocate user-context IDs, right? They could easily solve this by e.g. allocating PB user-context IDs in some separate number range. > What is the advantage of reusing userContextId here? Because it feels like a very sensible abstraction, especially if we want multiple PB as mentioned in comment 3. It seems to me that PB is a logical extension to be built on top of user contexts, leveraging all the same machinery and adding a few additional features like non-persistence and disabling visited handling.

Backing up a bit, I think the basic issue here is that implementing it either way is pretty trivial, and the key question comes down to what abstraction we want. Is PB a user context? Or does PB "live inside" a user-context (i.e. each user context has one PB and one non PB context)? The former feels more sensible to me, which is why I'm proposing this model.

(In reply to Bobby Holley (busy) from comment #10) > Is PB a user context? No. To a user they are quite different. You open a private-browsing window by clicking a menu item. This creates a temporary window with data, and this data goes away when the window is closed. User-contexts are managed through a user-context manager. You create and destroy them in this user-context manager. When inspecting cookies and other stored data, the UI lets you see which user-context they belong to. You open a user-context tab similarly to how you open a new tab. I forget the details, but I believe that it either involves right-clicking the new-tab button, or right-clicking a tab. This tab renders in the same window as all your "normal" tabs, but the tab itself has some decoration indicating that it belongs to a non-default context. > Or does PB "live inside" a > user-context (i.e. each user context has one PB and one non PB context)? No. I think that for now PB lives entirely in parallel to user-contexts. Someone working on userContext should confirm the above information. Adding Kate who might be able to confirm, or at least know someone who is. To be clear. I'm not arguing that PB and userContexts are orthogonal. I believe they are different. Similar to how userContext and addonid are not orthogonal but rather different.

(In reply to Jonas Sicking (:sicking) from comment #11) > (In reply to Bobby Holley (busy) from comment #10) > > Is PB a user context? > > No. > > To a user they are quite different. You open a private-browsing window by > clicking a menu item. This creates a temporary window with data, and this > data goes away when the window is closed. I understand that to the _user_ they are different. The question I'm asking is how we want to implement them (including the front-end bits that manage PB and user context state). I am suggesting that there are a lot of efficiencies to be derived from implementing PB on top of user contexts.

The UX for the two seems entirely different, as detailed in comment 11. So I would assume that the implementation of the UX would be entirely different.

> I am suggesting that there are a lot of efficiencies to be derived from implementing PB on top of user contexts. Can you detail what efficiencies you are referring to? The main one I see is saving 4 bytes in the OA struct.

(In reply to Jonas Sicking (:sicking) from comment #14) > > I am suggesting that there are a lot of efficiencies to be derived from implementing PB on top of user contexts. > > Can you detail what efficiencies you are referring to? The main one I see is > saving 4 bytes in the OA struct. For most of the cases I can think of where we would handle user contexts in C++, it seems like we would also want to handle PB. A quick MXR search seems to indicate that we don't have a lot of special handling for user contexts in C++, which is great. But the ones I find seem like places where we're going to want to handle both: * Disabling isolation in permissions: http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#116 * The checks we do in NS_CompareLoadInfoAndLoadContext * All of the window-opening gunk in nsWindowWatcher.cpp It seems somewhat more likely that there's a long tail of places where we've segmented things for PB and we're eventually going to want to segment them for user contexts (but just haven't found them yet). I could be wrong though. There's also the issue of segmenting the history for visited highlighting (which is possibly not relevant if we want to disable visited highlighting entirely in PB even with this segmenting).

Anyway, I'm not going to be the one maintaining this, so if nobody else thinks it's a good idea to build PB on top of user contexts, I'm fine to drop it.

> * Disabling isolation in permissions: > http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/ > nsPermissionManager.cpp#116 For userContext's this one is temporary. Which means that eventually we'll have the opposite situation here. I.e. we'll separate permissions by userContext, but we will not want to do that for private-browsing. So putting private-browsing in userContextId will make things harder. > * The checks we do in NS_CompareLoadInfoAndLoadContext Ideally LoadContext should die and all of this could will go away with it. If that doesn't happen then LoadContext should start using OriginAttributes, and this will just become LoadContext.OriginAttributes == LoadInfo.OriginAttributes. So nothing userContextId-specific is expected to remain here. > * All of the window-opening gunk in nsWindowWatcher.cpp I can't comment on this since I don't know what this is. > It seems somewhat more likely that there's a long tail of places where we've > segmented things for PB and we're eventually going to want to segment them > for user contexts (but just haven't found them yet). I could be wrong though. Given how different user-contexts and PB is from a product point of view (i.e. in the UX), I think it's more likely that we'll end up with pain points by treating userContext as PB, than code reuse. See the permissions example earlier in this comment. > There's also the issue of segmenting the history for visited highlighting > (which is possibly not relevant if we want to disable visited highlighting > entirely in PB even with this segmenting). Indeed does not seem relevant. And if we do segment history visiting, we should do that generically for all OriginAttributes. It seems to me that PB is mostly similar to userContexts by the fact that both should affect originAttributes, rather than that they are in fact the same concept. Also add to the list of complexities of making PB user userContext that we'll have to do cross-process syncing of the PB-hashtable. That in and of itself seems to add more complexity than the sum of wins mentioned so far.

> What is the advantage of reusing userContextId here? From my point of view having privateBrowsing flag is _the_ hack. What we really want is: a _unique_ context that does't store data to disk at all, and that can be easily be removed. 'privateBrowsing' is a UX concept that, in platform becomes: a unique userContextId + inMemory. If we introduce this 'inMemory' flag, we can have multiple: . PrivateBrowsing sessions . Presentation API built on top of it . "guest" mode - probably this will be built on top of profiles, but it's a UX decision. . etc etc. We should try to have a better separation between UX and platform. Our code base has an extra level of complexity just because we want to have propagate this flag everywhere. If we have OriginAttribute.inMemory, QuotaManager can deal with this offering a virtual fileSystem. This virtual FileSystem will make IndexedDB, *Storage, etc APIs working without 1 single line of code changed.

I want NI Gijs, how recently helped me to implement many UI/UX features for containers. And Tanvi, who is the leader for the Container project, plus member of the security team.

(In reply to Andrea Marchesini (:baku) from comment #19) > I want NI Gijs, how recently helped me to implement many UI/UX features for > containers. OK, but what is the question? :-) I read through the rest of the bug... I can see arguments for both points of view wrt private browsing and userContextIds. It feels to me like it boils down to how analogous we think PB really is to userContextIds. If I have 2 user context IDs, let's say "Personal" and "Work", would I expect to have 2 different sets of private browsing sessions, one for each user context id? My gut feeling is "no"... When using private browsing the main expectation is that the session is private, and that there will be almost no traces of it left when the session finishes (nothing beyond bookmarks and downloads). There are cases where this becomes very tricky when mixed with permissions... (In reply to Jonas Sicking (:sicking) from comment #17) > > * Disabling isolation in permissions: > > http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/ > > nsPermissionManager.cpp#116 > > For userContext's this one is temporary. Which means that eventually we'll > have the opposite situation here. I.e. we'll separate permissions by > userContext, but we will not want to do that for private-browsing. Won't we? Does that mean you think if the user gives the site permission to store data in indexeddb / geolocate the user / use the user's camera / ... in normal mode, that permission should be there in private browsing mode, but *not* in a different user context id? That seems surprising to me - it makes different user context ids "more private" than private browsing. Maybe I misunderstand your point? > So putting private-browsing in userContextId will make things harder. Well, does it depend on the permission? There's ongoing confusion about what to do about e.g. HSTS (which might not be tracked in the permissions manager necessarily, but is conceptually similar): bug 1232961. If you persist HSTS from normal mode to private browsing mode, that's an identification vector that lets sites identify you despite being in private browsing. If you don't, that's a security vector that could compromise users' security. For other things, like indexeddb quotas or cookies or even camera/geolocation permissions, while it seems clear that the data itself should be separate (and transient) for private browsing usage, that is less clear to me that they should be separate for different usercontextids. If I allow some voip/webrtc app to send me notifications on my personal account, it seems reasonably likely I want them on my work account. Ultimately, a lot of this depends on the usecases we see for the different usercontextids and "how separate" we want to keep them. Likewise for private browsing. It doesn't feel like there's a clear consensus/direction on that point - though maybe there is and I'm just missing it?

Hmm, there seems to be quite some confusion in this bug. Let me try to summarize how things should work here. :-) It seems like Bobby is basically arguing for there to be a way to map a user context ID to whether it should be persisted or not. You can imagine this system to be a bit on top of the user context ID, so conceptually it's no different than what Andrea and I are proposing here (an explicit boolean flag in OriginAttributes). But implementation wise, the suggestion of having a UserContextMayPersist(userContextId) predicate is not a good idea for two reasons: * It's probably less efficient to implement assuming that the idea is to maintain a current list of user context IDs that must not be persisted. The best implementation that can be done is for example use the highest bit of context ID or some such instead of having a centralized list, which is essentially a boolean flag. * It has the much more severe downside of making some context IDs "special", which means that code that deals with user context IDs and can otherwise be oblivious to the persistence attribute will now need to take care to check the context ID for persistence too. One things that makes this complicated is that containers and private browsing will probably have some overlap but PB is not necessarily a subset of containers or vice versa. I totally expect there to be cases where what we want to do with containers (for example whether a push notification permission should work across containers, and whether it should work across private and non-private sessions, it's entirely possible for us to choose to have different UX decisions for these two cases.) Because of this, I think we *really* want to keep the persistence flag and the context IDs separate at all costs. This can also simplify the implementation of the front-end in cases where for example we want to use things such as context IDs in CSS selectors, etc. Also, as Andrea suggested, there are other use cases for creating non-persistent OA sub-spaces in a given context ID, such as the presentation API use case in the user's "Work" container. Having the persistence as a separate flag will enable that use case in a clean way, and will leave the room open for further UX changes such as treating each private window as a separate private session (internally implemented as non-persistent temporary containers) which is something that users have asked for a lot. Also, FWIW, I have an intern starting next week, and I'm planning to ask him to start working on re-implementing private browsing based on OA following this plan.

In the current code base, if you enable userContext in about:config and open private browsing mode, you will see a menu item for "Open New Container Tab". This will open a "Private Browsing Personal Tab" and a "Private Browsing Work Tab" etc. This is a very confusing UX that we plan to disable (https://bugzilla.mozilla.org/show_bug.cgi?id=1270471). It actually does work, and does create multiple different private browsing sessions. Safari has this concept, where each tab in private browsing mode is it's own segregated ephemeral session. We are considering something like this in the future, but not with the current UI! If we had a separate isPrivateMode OriginAttribute, then we could combine that with userContextId. So you could have have separate cookie jars for: isPrivateMode=false, userContextId=0 isPrivateMode=true, userContextId=0 isPrivateMode=false, userContextId=1 isPrivateMode=true, userContextId=1 I like this better than trying to segment the userContextId range, so that 0-50 are non-private user contexts and 51-100 are private user contexts (for example). That seems confusing to me. With two separate Origin Attributes, the UI could would have to check the isPrivateMode flag before checking userContextId. Once it knows whether we are in private mode, it can branch into the appropriate UI. (ex: userContextId=1 in non-private mode could have blue decoration with a thumbprint image, userContextId=1 in private mode could have the blue decoration with the blue mask image) I would like to see the private browsing boolean flags that are passed around our entire code base moved into an OriginAttribute of its own. This would be a good first step and is a lot of work in itself. But I'm not sure how that will translate over into an "inMemory" flag that is not always used with private browsing mode. (In reply to Bobby Holley (busy) from comment #9) > (In reply to Jonas Sicking (:sicking) from comment #8) > > This is needed when we turn off link coloring > > Don't we want link color to also be user-context-aware? This seems like a > case that would benefit from more-unified handling, though I understand that > we also have an additional more-stringent policy for PB that goes above and > beyond simply segmenting the histories. No. userContexts should segregate cookies/localstorage/indexeddb/http cache/image cache. Things that websites can use to identify you. They do not segregate history, bookmarks, passwords, visited links, Saved Search and Form data. Websites don't see this data, only the user does, so there is no need to segregate. UserContexts are intended to be used by one user, not multiple. So we aren't trying to hide information from anyone on the client side. Private browsing mode is different, because we want to hide information from both the website and the local client. (Note we are still debating how to handle HSTS, OCSP Repsonses, and certificate overrides, but at least for now they will not be segregated.) (In reply to Jonas Sicking (:sicking) from comment #17) > > * Disabling isolation in permissions: > > http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/ > > nsPermissionManager.cpp#116 > > For userContext's this one is temporary. Which means that eventually we'll > have the opposite situation here. I.e. we'll separate permissions by > userContext, but we will not want to do that for private-browsing. So > putting private-browsing in userContextId will make things harder. > Permissions will not be segregated by userContexts. At least for now. After a few rounds of user feedback we will see if this is something that users are asking for. (In reply to Bobby Holley (busy) from comment #15) > It seems somewhat more likely that there's a long tail of places where we've > segmented things for PB and we're eventually going to want to segment them > for user contexts (but just haven't found them yet). I could be wrong though. > In some cases, we have purposefully chosen not to segment. And yes, there may be other cases we haven't thought of yet.

(In reply to Tanvi Vyas [:tanvi] from comment #22) > I would like to see the private browsing boolean flags that are passed > around our entire code base moved into an OriginAttribute of its own. This > would be a good first step and is a lot of work in itself. But I'm not sure > how that will translate over into an "inMemory" flag that is not always used > with private browsing mode. The majority of private browsing implementation is based around deciding whether to persist some piece of data related to the user's browsing to disk, and those parts can be moved over to using the "inMemory" flag (a private sessions principals will have the inMemory flag set to true.) As we keep working on this project, we may come across cases where we do something more specific to private browsing than just choosing whether to persist some piece of data in memory. For example, we don't auto-fill the saved login info on pages which are loaded in a private session. For those cases we may need to have a different behavior than what "inMemory" gives us. In that case we would probably need to add a flag specific to private browsing (such as inPriavateBrowsing). In that case we need to preserve the invariant of inMemory always being true when inPrivateBrowsing is true through some assertions.

(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #23) > (In reply to Tanvi Vyas [:tanvi] from comment #22) > > I would like to see the private browsing boolean flags that are passed > > around our entire code base moved into an OriginAttribute of its own. This > > would be a good first step and is a lot of work in itself. But I'm not sure > > how that will translate over into an "inMemory" flag that is not always used > > with private browsing mode. > > The majority of private browsing implementation is based around deciding > whether to persist some piece of data related to the user's browsing to > disk, and those parts can be moved over to using the "inMemory" flag (a > private sessions principals will have the inMemory flag set to true.) > > As we keep working on this project, we may come across cases where we do > something more specific to private browsing than just choosing whether to > persist some piece of data in memory. For example, we don't auto-fill the > saved login info on pages which are loaded in a private session. For those > cases we may need to have a different behavior than what "inMemory" gives > us. In that case we would probably need to add a flag specific to private > browsing (such as inPriavateBrowsing). In that case we need to preserve the > invariant of inMemory always being true when inPrivateBrowsing is true > through some assertions. Thanks for clearing that up! In that case, we need another bug to create OriginAttributes.inPrivateBrowsing, that probably takes higher priority than this bug. Or we can add them at the same time here, ensuring that if(inPrivateBrowsing) { inMemory = true; }.

(In reply to Tanvi Vyas [:tanvi] from comment #24) > (In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment > #23) > > (In reply to Tanvi Vyas [:tanvi] from comment #22) > > > I would like to see the private browsing boolean flags that are passed > > > around our entire code base moved into an OriginAttribute of its own. This > > > would be a good first step and is a lot of work in itself. But I'm not sure > > > how that will translate over into an "inMemory" flag that is not always used > > > with private browsing mode. > > > > The majority of private browsing implementation is based around deciding > > whether to persist some piece of data related to the user's browsing to > > disk, and those parts can be moved over to using the "inMemory" flag (a > > private sessions principals will have the inMemory flag set to true.) > > > > As we keep working on this project, we may come across cases where we do > > something more specific to private browsing than just choosing whether to > > persist some piece of data in memory. For example, we don't auto-fill the > > saved login info on pages which are loaded in a private session. For those > > cases we may need to have a different behavior than what "inMemory" gives > > us. In that case we would probably need to add a flag specific to private > > browsing (such as inPriavateBrowsing). In that case we need to preserve the > > invariant of inMemory always being true when inPrivateBrowsing is true > > through some assertions. > > Thanks for clearing that up! In that case, we need another bug to create > OriginAttributes.inPrivateBrowsing, that probably takes higher priority than > this bug. Or we can add them at the same time here, ensuring that > if(inPrivateBrowsing) { inMemory = true; }. Yes, I think we can deal with the other flag in a separate bug. I filed bug 1271686 for that. I've actually asked James (who I CCed on the bug today!) to focus on the inMemory OriginAttributes value first because of the additional use case of the presentation API, but we'll probably look at both sort of in parallel as he starts to make progress on the implementation.

I talked with baku about this a few days ago, but none of that information made it into this bug, so I'll write the important bits here. userContextId is separate from private-browsing: I think there almost full agreement that designating some userContextIds as "private browsing" is *not* what we want to do. Neither by using ranges, using evens/odds, using a high-order bit, or by using a separate data store indicating which userContextIds are "private browsing" and which ones aren't. The two concepts are different and we should keep them different. Support for multiple private-browsing sessions: We will likely eventually want to support having multiple private-browsing sessions working at once. What the UI for this will look like remains to be determined, but doesn't really matter for now. The point is that we should make sure that the OriginAttributes format we use to represent private-browsing should at least be extensible in the future without too much of a headache. Orthogonality of OriginAttribute members OriginAttributes is currently designed to hold a number of "orthogonal" members. I.e. in theory any value for addonId can currently be combined with any value for userContextId. However in practice addonId is only ever set if userContextId is 0, and userContextId is only ever set if addonId is 0. And I believe that this will be a common pattern that we'll see. As we extend OriginAttributes, I don't think that we'll commonly see things that are truly orthogonal to all existing attributes. Instead we'll see more things that will want to create their own *additional* set of cookie jars. This is what private-browsing wants as well. It doesn't want something that combines with addons, or that combines with the "home" vs. "work" contexts. It wants an unbounded set of "private-browsing cookie jars" which it can identify through an id. The theoretically more pure way to represent this in OriginAttributes would be something like dictionary OriginAttributes { DOMString jarType; // "addon", "userContext" or "privateBrowsing" DOMString jarId; // meaning depends on value in jarType } But I think what we have is fine. Especially for now. If for no other reason than that they allow the different id's to be of different type. What I suggest we do: I suggest that we add dictionary OriginAttributes { bool privateBrowsing; // or 'unsigned long' }; Then add assertions which check that addonId, userContextId and privateBrowsing are mutually exclusive. If in the future we add something which truly is orthogonal to existing attributes, we simply don't assert that it is mutually exclusive. And if we in the future, say, allow different userContextIds to install different addons, and we want to separate addon data per userContextId, then we adjust the assertions to allow them to be set at the same time. I *do* think it's important that we keep the members of OriginAttributes having semantic meaning. Which is why I think having a 'privateBrowsing' member is better than a 'inMemory' member. That way we can do things like base whether we do link coloring by looking at the document.principal.originAttributes.privateBrowsing member.

On a separate topic, what is this bug intended to cover? Is the idea to track private-browsing-ness inside of OriginAttributes in addition to how we're currently tracking it? Does that mean that docshells will have two members to track private-browsing-ness? Or is the idea to replace the current tracking of private-browsing with something in DocShell? That will require a lot more than just adding members to OriginAttributes.

Comment 26 sounds reasonable to me.

(In reply to Jonas Sicking (:sicking) from comment #26) > I talked with baku about this a few days ago, but none of that information > made it into this bug, so I'll write the important bits here. > > userContextId is separate from private-browsing: > I think there almost full agreement that designating some userContextIds as > "private browsing" is *not* what we want to do. Neither by using ranges, > using evens/odds, using a high-order bit, or by using a separate data store > indicating which userContextIds are "private browsing" and which ones aren't. > > The two concepts are different and we should keep them different. > > Support for multiple private-browsing sessions: > We will likely eventually want to support having multiple private-browsing > sessions working at once. What the UI for this will look like remains to be > determined, but doesn't really matter for now. The point is that we should > make sure that the OriginAttributes format we use to represent > private-browsing should at least be extensible in the future without too > much of a headache. > > > Orthogonality of OriginAttribute members > OriginAttributes is currently designed to hold a number of "orthogonal" > members. I.e. in theory any value for addonId can currently be combined with > any value for userContextId. However in practice addonId is only ever set if > userContextId is 0, and userContextId is only ever set if addonId is 0. > > And I believe that this will be a common pattern that we'll see. As we > extend OriginAttributes, I don't think that we'll commonly see things that > are truly orthogonal to all existing attributes. Instead we'll see more > things that will want to create their own *additional* set of cookie jars. > > This is what private-browsing wants as well. It doesn't want something that > combines with addons, or that combines with the "home" vs. "work" contexts. > It wants an unbounded set of "private-browsing cookie jars" which it can > identify through an id. > > The theoretically more pure way to represent this in OriginAttributes would > be something like > > dictionary OriginAttributes { > DOMString jarType; // "addon", "userContext" or "privateBrowsing" > DOMString jarId; // meaning depends on value in jarType > } > > But I think what we have is fine. Especially for now. If for no other reason > than that they allow the different id's to be of different type. > > > What I suggest we do: > > I suggest that we add > dictionary OriginAttributes { > bool privateBrowsing; // or 'unsigned long' > }; Yeah agreed with everything here. When I talked to baku about this, he sort of convinced me that once we have temporary user context IDs, we can use them for implementing separate private windows, so that we wouldn't need to make privateBrowsing an integer here, but the good thing is that we can switch this from a boolean to an integer or the other way around pretty easily, so it doesn't matter that much which one to pick here. I guess we can use a boolean for now. > I *do* think it's important that we keep the members of OriginAttributes > having semantic meaning. Which is why I think having a 'privateBrowsing' > member is better than a 'inMemory' member. That way we can do things like > base whether we do link coloring by looking at the > document.principal.originAttributes.privateBrowsing member. Well, they're both useful, since the presentation API use case requires using an OriginAttributes that designates that nothing about that session should be persisted, but it doesn't want the rest of the PB behavior (such as visited link coloring). Therefore it's used to separate these two cases into two flags now and when updating the consumers, look at the correct bit depending on whether we're making a persistence decision vs. some other PB related decision.

(In reply to Jonas Sicking (:sicking) from comment #27) > On a separate topic, what is this bug intended to cover? Is the idea to > track private-browsing-ness inside of OriginAttributes in addition to how > we're currently tracking it? Does that mean that docshells will have two > members to track private-browsing-ness? > > Or is the idea to replace the current tracking of private-browsing with > something in DocShell? That will require a lot more than just adding members > to OriginAttributes. James is working on replacing the current tracking of PB with the OriginAttributes flag, which as you said is more work than just adding the OA flag. We'll probably use this bug as a tracker for this work.

> I guess we can use a boolean for now. I'm happy to use a |unsigned long privateBrowsingId;| right away. And just set it to 0 or 1 for now. > Well, they're both useful, since the presentation API use case requires > using an OriginAttributes that designates that nothing about that session > should be persisted, but it doesn't want the rest of the PB behavior (such > as visited link coloring). Therefore it's used to separate these two cases > into two flags now and when updating the consumers, look at the correct bit > depending on whether we're making a persistence decision vs. some other PB > related decision. If we want to support presentation API and private-browsing, then it seems like we'll need more than just a |bool inMemory;|, right? Do you have a proposal? It sounds to me like what presentation API wants is another example of an "additional set of cookie-jars that can be identified through an id". So the obvious option to me is to add |unsigned long presentationId| to OriginAttributes. With assertions preventing it from overlapping with the other ids of course. I agree that it has the disadvantage that it means that localStorage and permissions (and maybe some day quota-manager) has to check both mPrivateBrowsing(Id) and mPresentationId to check for in-memory-ness. But that seems ok, and could be abstracted into a helper.

Just keep in mind that OriginAttributes are designed in such a way as to assume that existing attributes will not disappear or change data type - that is to say we can add support for new attributes, but not easily remove support for existing ones (since this affects serialized data). So please be deliberate and forward-thinking when introducing new OAs.

As long as we change the de-serializing code to deal with the old format, isn't it safe to change datatype, or even do things like combine multiple attributes into one?

(In reply to Jonas Sicking (:sicking) from comment #33) > As long as we change the de-serializing code to deal with the old format, > isn't it safe to change datatype, or even do things like combine multiple > attributes into one? It'll break any consumers that expect to do string comparisons of origins and use that for equality, and combining existing OAs would break downgrading. It's certainly doable, but I'd rather avoid it if we can.

(In reply to Bobby Holley (busy) from comment #34) > (In reply to Jonas Sicking (:sicking) from comment #33) > > As long as we change the de-serializing code to deal with the old format, > > isn't it safe to change datatype, or even do things like combine multiple > > attributes into one? > > It'll break any consumers that expect to do string comparisons of origins > and use that for equality, and combining existing OAs would break > downgrading. It's certainly doable, but I'd rather avoid it if we can. In that case, lets make the private browsing mode Origin Attribute an unsigned long, that currently only takes a value of 0 or 1. We can assert that it is only 0 or 1, and remove the assertion in the future when we have multiple private browsing modes.

Comment on attachment 8755978 [details] [diff] [review] inMemory and inPrivateBrowsing patch Review of attachment 8755978 [details] [diff] [review]: ----------------------------------------------------------------- I'll want to see the changes to make the private browsing a numeric value instead of a boolean. Otherwise this looks reasonable, although I was confused that bug 1271686 is being ignored and we're adding both attributes in this one. ::: caps/BasePrincipal.cpp @@ +194,4 @@ > : mOriginAttributes(aOriginAttributes) > { > MOZ_ASSERT(aOriginAttributes); > + // If mInMeory or mInPrivateBrowsing are passed in as true and are not present in the suffix, mInMemory ::: caps/BasePrincipal.h @@ +184,5 @@ > + if (mInMemory.WasPassed() && mInMemory.Value() != aAttrs.mInMemory) { > + return false; > + } > + > + if (mInPrivateBrowsing.WasPassed() && mInPrivateBrowsing.Value() != aAttrs.mInPrivateBrowsing){ nit: space before { ::: docshell/base/nsDocShell.cpp @@ +3666,5 @@ > } > } > > +void > +nsDocShell::AssertPrivateBrowsing(bool aIsPrivate){ Let's call this AssertOriginAttributesMatchPrivateBrowsing. Why not have this using mInPrivateBrowsing, rather than accepting an argument? ::: docshell/base/nsDocShell.h @@ +1034,5 @@ > nsIDocShellTreeItem* aOriginalRequestor, > nsIDocShellTreeItem** aResult); > > + // Helper assertion to enforce that mInPrivateBrowsing is in sync with > + // OriginAttributes mInMemory and mInPrivateBrowsing OriginAttributes' ::: dom/base/nsFrameLoader.cpp @@ +3371,4 @@ > mOwnerContent->GetAttr(kNameSpaceID_None, > nsGkAtoms::mozpresentation, > presentationURLStr); > + nit: trailing whitespace ::: dom/ipc/TabChild.cpp @@ +830,5 @@ > nsCOMPtr<nsIDocShell> docShell = do_GetInterface(WebNavigation()); > MOZ_ASSERT(docShell); > > + nsDocShell::Cast(docShell)->SetOriginAttributes(OriginAttributesRef()); > + nit: trailing whitespace @@ -885,4 @@ > } > > UpdateFrameType(); > - nsDocShell::Cast(docShell)->SetOriginAttributes(OriginAttributesRef()); Why did this move? @@ +2381,4 @@ > if (!UpdateTabContextAfterSwap(maybeContext.GetTabContext())) { > MOZ_CRASH("Update to TabContext after swap was denied."); > } > + NotifyTabContextUpdated(); It's not clear to me why this is necessary, given the earlier code movement. ::: dom/ipc/TabContext.cpp @@ +160,4 @@ > return true; > } > > +bool nit: trailing whitespace ::: dom/ipc/TabContext.h @@ +154,4 @@ > bool SetTabContext(const TabContext& aContext); > > /** > + * Used to match the tab contexts origin attributes with private browsing I'm not sure what this comment means, since match is usually used in the sense of comparing values. ::: dom/ipc/TabParent.cpp @@ +2773,4 @@ > if (mLoadContext) { > loadContext = mLoadContext; > } else { > + SetPrivateBrowsingAttributes(mChromeFlags & nsIWebBrowserChrome::CHROME_PRIVATE_WINDOW); Let's add a local variable for this value and use it below as well. ::: dom/tests/mochitest/localstorage/test_localStorageQuotaPrivateBrowsing_perwindowpb.html @@ +140,5 @@ > case "perf": > + // postMessage should send to the slaveOrigin. However with the addition of private > + // browsing flags in origin attributes this will cause postMessage to fail. The origin of this > + // window has false privatebrowsing, while the recipient is in a private window. > + // To fix this issue and preserve the integrity of the test a * is passed to get around origin equality. nit: remove the trailing space here. ::: dom/webidl/ChromeUtils.webidl @@ +80,5 @@ > boolean inIsolatedMozBrowser = false; > DOMString addonId = ""; > DOMString signedPkg = ""; > + boolean inMemory = false; > + boolean inPrivateBrowsing = false; This should be unsigned long instead, per recent comments in this bug. ::: netwerk/test/TestCookie.cpp @@ +743,4 @@ > // Stubs to make this test happy > > mozilla::dom::OriginAttributesDictionary::OriginAttributesDictionary() > + : mAppId(0), mInIsolatedMozBrowser(false), mInMemory(false), mInPrivateBrowsing(false), mUserContextId(0) Let's split these across multiple lines.

Comment on attachment 8757485 [details] [diff] [review] TwoNewOAttrs.diff Review of attachment 8757485 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsDocShell.cpp @@ +14199,5 @@ > + } > + > + if (attrs.mInPrivateBrowsing.WasPassed()) { > + mOriginAttributes.mInPrivateBrowsing = attrs.mInPrivateBrowsing.Value(); > + } As I said in another bug, I prefer to have all of these, in a ::Set() method in OriginAttributesPattern.

Comment on attachment 8757485 [details] [diff] [review] TwoNewOAttrs.diff Review of attachment 8757485 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/BasePrincipal.cpp @@ +259,5 @@ > + if (aName.EqualsLiteral("inPrivateBrowsing")) { > + nsresult rv; > + int64_t val = aValue.ToInteger64(&rv); > + NS_ENSURE_SUCCESS(rv, false); > + NS_ENSURE_TRUE(val <= UINT32_MAX, false); We should check that it's >= 0 as well. ::: docshell/base/nsDocShell.cpp @@ +14166,4 @@ > nsDocShell::SetOriginAttributes(JS::Handle<JS::Value> aOriginAttributes, > JSContext* aCx) > { > + OriginAttributesPattern attrs; What's the motivation for this change? ::: dom/ipc/TabChild.cpp @@ -885,4 @@ > } > > UpdateFrameType(); > - nsDocShell::Cast(docShell)->SetOriginAttributes(OriginAttributesRef()); I don't think my question about why this code is moving was answered. @@ +2385,4 @@ > // Since mIsMozBrowserElement may change in UpdateTabContextAfterSwap, so we > // call UpdateFrameType here to make sure the frameType on the docshell is > // correct. > + NotifyTabContextUpdated(); It's not clear to me what this change accomplishes. ::: dom/ipc/TabContext.h @@ +154,4 @@ > bool SetTabContext(const TabContext& aContext); > > /** > + * Set the tab contexts origin attributes to a private browsing value. context's @@ +155,5 @@ > > /** > + * Set the tab contexts origin attributes to a private browsing value. > + */ > + bool SetPrivateBrowsingAttributes(bool aIsPrivateBrowsing); This doesn't need to return anything. ::: dom/webidl/ChromeUtils.webidl @@ +80,5 @@ > boolean inIsolatedMozBrowser = false; > DOMString addonId = ""; > DOMString signedPkg = ""; > + boolean inMemory = false; > + unsigned long inPrivateBrowsing = 0; This should be called privateBrowsingId. @@ +89,5 @@ > boolean inIsolatedMozBrowser; > DOMString addonId; > DOMString signedPkg; > + boolean inMemory; > + unsigned long inPrivateBrowsing; Same as above. ::: extensions/cookie/nsPermissionManager.cpp @@ +104,4 @@ > namespace { > > nsresult > +GetOriginFromPrincipal(nsIPrincipal* aPrincipal, nsACString & aOrigin) nit: nsACString& @@ +113,5 @@ > + rv = aPrincipal->GetOriginSuffix(suffix); > + NS_ENSURE_SUCCESS(rv, rv); > + > + mozilla::PrincipalOriginAttributes attrs; > + if(!attrs.PopulateFromSuffix(suffix)){ nit: space before ( and { ::: netwerk/base/LoadContextInfo.cpp @@ +131,2 @@ > NeckoOriginAttributes oa; > NS_GetOriginAttributes(aChannel, oa); We should modify NS_GetOriginAttributes to set the private browsing values appropriate when passed a channel, since we can use NS_UsePrivateBrowsing.

NotifyTabContext was changed with a merge. However, I believe that patch was backed out and I can't see any logical reason to change it, so I reverted the change. SetOriginAttributes was changed because of https://bugzilla.mozilla.org/show_bug.cgi?id=1274689 I removed that change from this patch.

Comment on attachment 8758047 [details] [diff] [review] TwoNewOAttrs.diff Review of attachment 8758047 [details] [diff] [review]: ----------------------------------------------------------------- Solid work! ::: caps/BasePrincipal.cpp @@ +257,5 @@ > + } > + > + if (aName.EqualsLiteral("privateBrowsingId")) { > + nsresult rv; > + int64_t val = aValue.ToInteger64(&rv); nit: extra space before = @@ +260,5 @@ > + nsresult rv; > + int64_t val = aValue.ToInteger64(&rv); > + NS_ENSURE_SUCCESS(rv, false); > + NS_ENSURE_TRUE(val >= 0 && val <= UINT32_MAX, false); > + mOriginAttributes->mPrivateBrowsingId = static_cast<uint32_t>(val); nit: extra space before = ::: extensions/cookie/nsPermissionManager.cpp @@ +117,5 @@ > + if (!attrs.PopulateFromSuffix(suffix)) { > + return NS_ERROR_FAILURE; > + } > + > + // mInMemory and mInPrivateBrowsing must be set to false because PermissionManager is not supposed to mPrivateBrowsingId

Fixed a merge conflict in nsFrameLoader.cpp (re-declaration of rv)

needs dom peer review: WebIDL file dom/webidl/ChromeUtils.webidl altered without DOM peer review

Peer Review for dom/webidl/ChromeUtils.webidl

Wait, does this patch add a mInMemory member to OA? I thought that there was agreement that mPrivateBrowsingId was enough?

We discussed this at length on IRC yesterday and today, and now everybody is on the same page. Here is a summary. The presentation API doesn't have any specific reason to avoid persisting its cookie jar when running a presentation. All we need to do for it is to provide the presentation with a brand new cookie jar that will get discarded when the presentation is finished, but whether or not that cookie jar is persisted to disk isn't important (as long we get rid of it when the presentation is finished!) Without the presentation API, the only use case for avoiding persistence would be private browsing, so we don't need to have a separate inMemory attribute on OriginAttributes just for that, we can just check the privateBrowsingID against 0. I've asked James to remove the inMemory attribute in this bug based on that. If in the future we find another use case for a non-persistent cookie jar, we can add something to OA for that, but we'll cross that bridge when we come to it.

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff298d2993a3 Add mPrivateBrowsingId to OriginAttributes r=ehsan,jdm

This was backed out from Firefox 49 in bug 1297687.